-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
similar(::SparseMatrixCSC, dims) returns sparse matrix with empty space (#26560) #30435
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a detail that will not be relevant to most users. Let's remove it from NEWS.
I removed the line in |
I misunderstood this originally. I was under the impression that this would only prevent the allocation of extra memory in the case that the input matrix's rowval and nzval had allocated more memory in these data structures past the end of colptr[end+1]. IIUC, this PR will always allocate zero memory for nzval and rowval (or am I misunderstanding the PR?). I suspect this could actually end up being a major breaking change in the package ecosystem. |
@ViralBShah, yes it could, but only, if the package ecosystem relies on a mis-feature. IMHO maintaining the invariant BTW, I did not hear of any example, where the feature is used in an existing package. |
I do know that people like the ability to use the internal representation of the sparse arrays. I agree that this is not a visible/documented API. But the question is, what does a user expect from I think that giving the same nonzero structure (basically colptr being the same as input) is a more reasonable default for sparse The only way to know if packages depend on this is through pkgeval. Not sure how easy it is to use. |
FWIW, I am another user who were very surprised that The only reason why I didn't create a PR myself was that I thought strict application of semver would not allow this change. But if Julia devs are OK with it would be very nice to have this change. |
Hi @ViralBShah, I agree with you, this functionality is present, in Currently, in the case, there are dimension arguments 2 cases are distinguished:
Your suggestion On important drawback of the current situation is, that if you want a IIRC a "breaking change" happens only if the documented behaviour changes. As |
IIRC this behavior is intentional and exploited by e.g. sparse broadcast to avoid repeated allocation; ref. |
I'm in favor of this change and There are two cases here. Either folks are:
So I'd say we go whole-hog here and always return empty storage, regardless of the |
After a bit of memory dredging, the rationale I recall for the present design is roughly the following: At some point consensus formed that, for sparse/structured/annotated matrices and vectors, For example, for (Fun related aside: Part of the eventual consensus was that Another example is that for That is, This information-preserving behavior is useful, for example in generic programming: When you send these special Absent this design, on a coarse level you end up with generic operations that, e.g., when called on On a finer level, and concerning the particular case at hand, you might end up with e.g. more allocations being necessary than you might expect. For example, suppose that some method one way or another does
The On the other hand, if you want an empty sparse matrix with the same eltype and/or shape as an existing sparse matrix, you can express that explicitly. I'm not sure that we should throw out the benefits of the present |
I would like to contradict @Sacha0 with all necessary respect, whence I am also a fan of consistency in design and API.
|
First, happy new year everyone! 🎉 All the best to you and yours in the coming year! :) Regarding
and
It might be worth noting that, regrettably, additional wrinkles exist. For example, given that we tend to avoid storing numerical zeros (and especially previously unstored numerical zeros), you do not always know how much storage you will need at the outset of an operation; you may have to complete the entire operation before you know. That complicates best and worst case analysis of allocation behavior. Take sparse broadcast again as a prime example, which (in the cases it was designed for) allocates only if it runs out of storage in the destination: You do not know how much storage e.g. Regarding
Yes, this unfortunate inconsistency was forced by the present design of Regarding
Agreed! :) We should at least add a guard capping the length of the buffers appropriately. Regarding
I'm not sure that first-touch performance via Regarding
Nicer ways to spell "create an empty sparse matrix" may indeed be in order. Again that's something of an orthogonal issue, and doesn't seem like the best motivation for a breaking change that removes useful functionality that cannot be otherwise readily achieved. Best! |
All the best to everyone :-)
Wouldn't it suffice to |
@Sacha0, Happy new year again! All the best to you and yours in the coming year! :)
I agree with that statement. Although it does not contradict my statements. I only say, that in one case we allocate a lot of memory - and it may be wrong, in the other case we allocate zero memory - which may be wrong as well. My argument is, that the expected cost is much higher in the first case.
I think, that is not a use case for
Also
But why should somebody prefer that over the simpler
The two additional empty allocations are correct in the case, which I refer to, when after
IMHO that is not a good idea. BTW this decoupling could also be done less visible to the API user by using
Of course not! It was just the simplest example that came into my mind, when I tried to demonstrate the performance burden involved by the big buffer, which is moved around. You could argue, that
I don't propagate this, in contrary. Actually I am very concerned about the bad performance of linear algebra operations of wrapped sparse matrix operands. That is a consequence of using the "abstract array fallback" AKA
Again, I could be easily convinced by one concrete example, which demonstrates the usefulness of the current implementation of Best regards, |
Just another example:
Here the time factor is only 17. Look also at memory allocation - while the count increases as predicted the total amount is much less (again no use is made of the existing buffer in the BEFORE case). |
What is the status of this PR? It would be good to have this in. |
As the discussion suggests, we do not have consensus here. I'm going to suggest leaving this open. @abraunst Is there a particular reason you would like it merged? The benchmark above is not convincing because the intended use case of |
I think that the current situation is bad (in particular having |
The "intended use case" is not documented, though. So the user, aware of the docs, is maybe not expecting this. Of course, the user can avoid using |
Yes, I should clarify, it was just my intention. I agree that at the very least it should be documented clearly. |
There seems to be consensus on approved JuliaSparse/SparseArrays.jl#44, which partially supersedes this PR. So I close this one. |
Hi all! Though this issue was closed in the interim, I finally have some time to respond and thought recording some thoughts might be useful for posterity: Regarding
and
and
and
Please reread #30435 (comment) (particularly the last two paragraphs) and #30435 (comment). As I explain there, it's not in the implementation of sparse broadcast that
Note that those
When working with preallocated storage, to avoid unnecessary allocation and memory overhead?
Decoupling of buffer length and number of stored entries is a well established property of compressed sparse data structures, and is regularly exploited in sparse linear algebra libraries. I will say more on this front in the relevant new thread later, but for now here's some previous discussion on the topic: #20464 (comment) .
The trouble we previously ran into with this approach is that the actual buffer sizes are then not exposed, and exposure of the buffer sizes to mutating methods is what allows those methods to avoid making guesses about what the size of the result should be. (Ref. e.g. the first couple paragraphs of
I both very much appreciate and am well aware of your work on things sparse, having reviewed much of that work :). Please consider reading comments charitably by default. Best! |
I would like to reinforce the comment about being extremely appreciative of the contributions. |
Sorry @Sacha0, I hope you forgive me for testing your patience. I may be bold, but I think I'm missing the point somehow.
In the first couple of paragraphs in #30435 (comment) the example is Could you give an example in which I think that this issue has potentially a large performance impact (in any way it is resolved). So maybe the best would be to propose concrete benchmarks (to add to the benchmark set) so we can test each of the approaches. What do you think? Otherwise, this issue will keep coming up IMHO... |
Actually only
That is true, but only once! That means, if |
The spirit of the is completely covered by JuliaSparse/SparseArrays.jl#44. |
As discussed in issue #26560 the reservation of space for
rowval
andnzval
in the same size as for the origin leads to inaccessible (by the exported API) memorylocations.
The behaviour now is the same for matrices as for sparse vectors in this respect.
It was checked, that in
stdlib
no use is made of this space (by modifying the internal fields). Actually nosimilar
of sparse matrices or vectors at all is used there.